Greptile Summary

This PR adds libp2p::identify protocol support to fix interop with gean (go-libp2p), which silently excludes peers that don't respond to /ipfs/id/1.0.0 during gossipsub GRAFT. The implementation mirrors zeam and ream: the behaviour is registered so the library responds to identify probes automatically, and all resulting events are intentionally discarded via the existing catch-all trace branch.

Confidence Score: 4/5

Safe to merge; the change is minimal, well-documented, and structurally correct.

Only P2 findings are present. The logic is sound — identify is correctly wired into the behaviour, the identity keypair refactor is clean, and events are intentionally ignored as documented. The single P2 is a style note about the protocol version string.

No files require special attention.

Sequence Diagram

sequenceDiagram
    participant gean as gean (go-libp2p)
    participant ethlambda as ethlambda swarm

    gean->>ethlambda: /ipfs/id/1.0.0 identify probe
    Note over ethlambda: identify::Behaviour<br/>responds automatically
    ethlambda-->>gean: IdentifyInfo (public key, /ipfs/0.1.0, listen addrs)
    Note over gean: identify exchange complete —<br/>peer allowed into gossipsub mesh
    gean->>ethlambda: gossipsub GRAFT
    ethlambda->>gean: gossipsub GRAFT

This is a comment left during a code review.
Path: crates/net/p2p/src/lib.rs
Line: 167

Comment:
**Protocol version string is ambiguous — consider a more descriptive value**

The `protocol_version` field in `identify::Config::new` is the *agent/application version* advertised to remote peers in the identify response — it is not the protocol negotiation path (`/ipfs/id/1.0.0`, which libp2p handles internally). Using a bare `/ipfs/0.1.0` may cause gean or other peers to misidentify this node as a generic IPFS node. Ethereum consensus clients typically advertise a more specific string (e.g. `/eth2/1.0.0` or a project-specific path). If zeam intentionally uses `/ipfs/0.1.0` for compatibility reasons that's fine to keep, but it's worth a comment explaining why this value was chosen over a project-specific one.

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (1): Last reviewed commit: "feat: add identify protocol support" | Re-trigger Greptile}

🤖 Kimi Code Review

Overall Assessment: Correct and idiomatic libp2p integration for go-libp2p interop. No security vulnerabilities identified.

Specific Comments:

crates/net/p2p/src/lib.rs

• Lines 166-169: The protocol version string "/ipfs/0.1.0" is semantically misleading for an Ethereum consensus client. While the comment correctly notes this matches zeam, add an explicit warning that this string is required for go-libp2p gossipsub GRAFT compatibility and should not be changed to an Ethereum-specific string without verifying peer connectivity with gean/zeam nodes.

• Line 72-76: The comment "Events from this behaviour are intentionally not handled" is slightly ambiguous. Consider clarifying: "Application logic intentionally ignores identify events; the behaviour auto-responds to probes at the libp2p swarm level."

• Lines 161-162: The expect on invalid node keys will panic during startup. While this is existing code moved from below, consider noting in a TODO that this should return a Result for graceful configuration error handling in production deployments.

• Line 166: Consider extracting "/ipfs/0.1.0" to a named constant (e.g., GO_LIBP2P_INTEROP_VERSION) to prevent accidental modification that would break peer connectivity.

Security & Consensus Notes:

• Acceptable: The identify protocol exposes the node's public key and supported protocols to all peers. This is standard for libp2p and necessary for the mesh formation described. No private key material is leaked.
• Network Health: This change correctly addresses silent graft failures with go-libp2p peers, which could otherwise cause network partitioning (isolation from the majority of the validator set).

Verdict: Approve with minor documentation suggestions.

Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt

🤖 Claude Code Review

Now I have everything I need to write a thorough review.

PR #327 — Add identify protocol support

Overview

This PR registers libp2p::identify::Behaviour in the Behaviour composite so that ethlambda responds to /ipfs/id/1.0.0 probes. The motivation is clear: gean (go-libp2p) gates gossipsub GRAFT on the identify exchange completing, causing ethlambda peers to be silently excluded from the mesh. The change is minimal and targeted.

Code Quality

Positive:

• The doc comment on Behaviour is excellent. It explains why identify is included and why events are intentionally ignored — exactly the kind of non-obvious reasoning that warrants a comment. Other files in the codebase follow this discipline too.
• Moving secret_key and identity construction up before Behaviour is a clean, necessary refactor. The original placement after the Behaviour block was already a bit awkward (the identity was only used by SwarmBuilder); now it correctly feeds both identify::Behaviour and SwarmBuilder::with_existing_identity.
• identity.public() returns a cloned PublicKey, so identity (the keypair) remains owned and is validly consumed by SwarmBuilder::with_existing_identity on the next line.

Concerns and Observations

1. protocol_version string vs. identify protocol ID

The code uses /ipfs/0.1.0 as the protocol_version argument to libp2p::identify::Config::new. This is the application-level protocol version field sent in the identify payload — not the identify protocol ID itself (which is always /ipfs/id/1.0.0 regardless). These are different things.

The protocol_version field is informational and most peers ignore it for accept/reject decisions. Using zeam's value is reasonable. However, it's worth confirming that ream and gean also accept peers with arbitrary protocol_version values (i.e., they don't filter on it). The doc comment mentions /ipfs/id/1.0.0 (the protocol ID) and the inline comment mentions /ipfs/0.1.0 (the payload version field) — a reader could confuse the two. A one-line note clarifying the distinction would help future maintainers.

2. Identify events flow to wildcard _ arm — intentional, but worth noting

BehaviourEvent::Identify events (received, pushed, error) are silently dropped via the _ => { trace!(?event, "Ignored swarm event") } arm in handle_swarm_event. This is intentional and correctly documented. One thing the current implementation gives up:

• Identify responses include the remote's observed address for you. Handling IdentifyEvent::Received could help with external address discovery / NAT traversal. For QUIC over devnet/LAN this is irrelevant, but it could matter for public mainnet.

No action needed now, but worth a // TODO: handle Identify::Received for external address discovery when needed comment if public network support is a future goal.

3. push_listen_addr_updates defaults to false

libp2p::identify::Config::new(...) uses default settings, including push_listen_addr_updates: false. This means ethlambda will respond to incoming identify requests but will not proactively push address updates to peers when its listen addresses change. This is the correct and conservative behavior for the stated goal.

4. Dependency — features = ["full"] already covers identify

The libp2p dependency in crates/net/p2p/Cargo.toml uses features = ["full"], so libp2p::identify was already available. No Cargo.toml change is needed, and none is missing from the diff.

5. Event queue noise at scale

With identify enabled, every new peer connection generates at minimum one IdentifyEvent::Received event. These are forwarded through the swarm event channel to the P2P actor and handled instantly (trace log only). No backpressure concern for expected peer counts in this network.

Minor Style Note

The inline comment // Use the same protocol_version string as zeam (backtick formatting inside a // comment) is fine but slightly inconsistent with the codebase's doc comment style. Not a blocker.

Summary

The change is correct, minimal, and well-motivated. The #[derive(NetworkBehaviour)] macro automatically adds BehaviourEvent::Identify to the event enum, the wildcard arm correctly swallows those events, and the identify handshake will satisfy gean's probing requirement. The only genuine suggestion is a clarifying note distinguishing the protocol_version payload field from the identify protocol ID — the current doc comment conflates them slightly in a way that could confuse future readers.

Automated review by Claude (Anthropic) · sonnet · custom prompt

🤖 Codex Code Review

Findings

• No blocking findings. The diff is isolated to libp2p swarm setup in lib.rs:69 and lib.rs:161; it does not touch fork choice, attestation validation, justification/finalization, state transition, XMSS, or SSZ paths.
• Reusing the same node key for both identify and SwarmBuilder::with_existing_identity looks correct and preserves peer-id stability (lib.rs:161, lib.rs:179).

Non-blocking Note

• Consider adding an explicit BehaviourEvent::Identify(...) arm near lib.rs:443 instead of relying on the generic ignore path at lib.rs:564. The current code is functionally fine, but swallowing all identify events makes interop failures harder to diagnose if this handshake ever regresses.

Testing Gap

• I could not run cargo check in this sandbox. The workspace depends on an uncached git dependency, lean-multisig, and offline compilation stops before reaching this crate.
• Since this PR is explicitly an interop fix, I’d still want one live smoke test against a go-libp2p/gean peer to confirm that identify completion actually results in gossipsub mesh admission.

Automated review by OpenAI Codex · gpt-5.4 · custom prompt